SmartPtr: Use shared ptr to manage GB objects. v6.0.126 #4080
Conversation
| // SrsSharedPtr<MyClass> cp = ptr; | ||
| // cp->do_something(); | ||
| template<class T> | ||
| class SrsSharedPtr |
There was a problem hiding this comment.
It seems SrsSharedPtr shared similar api with std::shared_ptr, that's good, so why not add a compiling option to control whether use a template alias from std::shared_ptr or self defined shared ptr.
| class SrsSharedPtr | |
| #ifdef _USE_STD_SMART_PTR | |
| template <class T> | |
| using SrsSharedPtr = std::shared_ptr<T>; | |
| #else | |
| template <class T> | |
| class SrsSharedPtr { | |
| // current definition | |
| }; | |
| #endif |
The benefit is to quickly workaround the shared ptr bugs in the future, e.g. the ref_count_ is not atomic, it's ok for current single-thread coroutine solution, but what if multi-thread coroutine are implemented in the future, then it's disaster. For the basic level infrastructure codes, it's a smart idea to always has a replacement solution.
There was a problem hiding this comment.
Won't fix. Using std::shared_ptr does not provide any benefits because if SrsSharedPtr has a bug, we would need to fix it.
TRANS_BY_GPT4
| @@ -81,4 +81,103 @@ class impl_SrsAutoFree | |||
| } | |||
| }; | |||
|
|
|||
| // Shared ptr smart pointer, see https://github.com/ossrs/srs/discussions/3667#discussioncomment-8969107 | |||
There was a problem hiding this comment.
Not a problem of this header file srs_core_autofree.hpp, but the empty srs_core_autofree.cpp, there is no reason to keep a empty cpp file except to waste compiler power.
There was a problem hiding this comment.
To save on compilation time, please provide the specific amount of time saved by reducing one cpp file.
This is unrelated to the current PR; please submit a separate PR for this matter.
TRANS_BY_GPT4
trunk/src/app/srs_app_gb28181.cpp
Outdated
| return err; | ||
| } | ||
| // Wait for the SIP connection to be terminated. | ||
| while (true) { |
There was a problem hiding this comment.
This while loop never break except return, is it a problem?
|
|
||
| if ((err = resource->start()) != srs_success) { | ||
| srs_freep(conn); | ||
| SrsExecutorCoroutine* executor = new SrsExecutorCoroutine(_srs_gb_manager, conn, raw_conn, raw_conn); |
There was a problem hiding this comment.
I think executor instance here is a memory leak. Here is the reason:
SrsExecutorCorountine is an ISrsResource which means it can use an ISrsResourceManager to manager its lifecycle, which is _srs_gb_manager in this case, check codes in SrsExecutorCoroutine::cycle():
srs_error_t SrsExecutorCoroutine::cycle()
{
srs_error_t err = handler_->cycle();
if (callback_) callback_->on_executor_done(this);
manager_->remove(this);
return err;
}
We have manager_->remove(this);, but where is the _srs_gb_manager->add(executor);?
And Check anther two SrsExecutorCoroutine instances, for SrsLazyGbMediaTcpConn and SrsLazyGbSession, they have same problem.
I didn't running any memory leak detector to confirm yet, but I think valgrind can detect it.
There was a problem hiding this comment.
Seems no problem. Won't fix.
winlinvip
force-pushed
the
feature/smart-ptr
branch
from
5c7303e
to
3088ff6
Compare
winlinvip
changed the title
winlinvip
changed the title
|
See #3667 (comment) |
The object relations:
Session manages SIP and Media object using shared resource or shared ptr. Note that I actually use SrsExecutorCoroutine to delete the object when each coroutine is done, because there is always a dedicate coroutine for each object.
For SIP and Media object, they directly use the session by raw pointer, it's safe because session always live longer than session and media object.